-
-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: add MFA for WebAuthn bindings #960
base: master
Are you sure you want to change the base?
Conversation
credential_request_options: { publicKey: PublicKeyCredentialRequestOptionsJSON } | ||
} | ||
error: null | ||
} | ||
| { data: null; error: AuthError } | ||
|
||
export type AuthMFAListFactorsResponse = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decide whether to include aaguid to image / authenticator type mapping for user: https:/passkeydeveloper/passkey-authenticator-aaguids
or to include it in documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs is better, we can add that in a follow up based on feedback.
src/lib/types.ts
Outdated
export interface AuthenticatorAttestationResponseJSON { | ||
clientDataJSON: Base64URLString | ||
attestationObject: Base64URLString | ||
// Optional in L2, but becomes required in L3. Play it safe until L3 becomes Recommendation | ||
authenticatorData?: Base64URLString | ||
// Optional in L2, but becomes required in L3. Play it safe until L3 becomes Recommendation | ||
transports?: AuthenticatorTransportFuture[] | ||
// Optional in L2, but becomes required in L3. Play it safe until L3 becomes Recommendation | ||
publicKeyAlgorithm?: COSEAlgorithmIdentifier | ||
publicKey?: Base64URLString | ||
} | ||
|
||
/** | ||
* A slightly-modified AuthenticatorAssertionResponse to simplify working with ArrayBuffers that | ||
* are Base64URL-encoded in the browser so that they can be sent as JSON to the server. | ||
* | ||
* https://w3c.github.io/webauthn/#dictdef-authenticatorassertionresponsejson | ||
*/ | ||
export interface AuthenticatorAssertionResponseJSON { | ||
clientDataJSON: Base64URLString | ||
authenticatorData: Base64URLString | ||
signature: Base64URLString | ||
userHandle?: Base64URLString | ||
} | ||
|
||
/** | ||
* A WebAuthn-compatible device and the information needed to verify assertions by it | ||
*/ | ||
export type AuthenticatorDevice = { | ||
credentialID: Base64URLString | ||
credentialPublicKey: Uint8Array | ||
// Number of times this authenticator is expected to have been used | ||
counter: number | ||
// From browser's `startRegistration()` -> RegistrationCredentialJSON.transports (API L2 and up) | ||
transports?: AuthenticatorTransportFuture[] | ||
} | ||
|
||
/** | ||
* An attempt to communicate that this isn't just any string, but a Base64URL-encoded string | ||
*/ | ||
export type Base64URLString = string | ||
|
||
/** | ||
* AuthenticatorAttestationResponse in TypeScript's DOM lib is outdated (up through v3.9.7). | ||
* Maintain an augmented version here so we can implement additional properties as the WebAuthn | ||
* spec evolves. | ||
* | ||
* See https://www.w3.org/TR/webauthn-2/#iface-authenticatorattestationresponse | ||
* | ||
* Properties marked optional are not supported in all browsers. | ||
*/ | ||
export interface AuthenticatorAttestationResponseFuture extends AuthenticatorAttestationResponse { | ||
getTransports(): AuthenticatorTransportFuture[] | ||
} | ||
|
||
/** | ||
* A super class of TypeScript's `AuthenticatorTransport` that includes support for the latest | ||
* transports. Should eventually be replaced by TypeScript's when TypeScript gets updated to | ||
* know about it (sometime after 4.6.3) | ||
*/ | ||
export type AuthenticatorTransportFuture = | ||
| 'ble' | ||
| 'cable' | ||
| 'hybrid' | ||
| 'internal' | ||
| 'nfc' | ||
| 'smart-card' | ||
| 'usb' | ||
|
||
/** | ||
* A super class of TypeScript's `PublicKeyCredentialDescriptor` that knows about the latest | ||
* transports. Should eventually be replaced by TypeScript's when TypeScript gets updated to | ||
* know about it (sometime after 4.6.3) | ||
*/ | ||
export interface PublicKeyCredentialDescriptorFuture | ||
extends Omit<PublicKeyCredentialDescriptor, 'transports'> { | ||
transports?: AuthenticatorTransportFuture[] | ||
} | ||
|
||
/** */ | ||
export type PublicKeyCredentialJSON = RegistrationResponseJSON | AuthenticationResponseJSON | ||
|
||
/** | ||
* A super class of TypeScript's `PublicKeyCredential` that knows about upcoming WebAuthn features | ||
*/ | ||
export interface PublicKeyCredentialFuture extends PublicKeyCredential { | ||
type: PublicKeyCredentialType | ||
// See https:/w3c/webauthn/issues/1745 | ||
isConditionalMediationAvailable?(): Promise<boolean> | ||
// See https://w3c.github.io/webauthn/#sctn-parseCreationOptionsFromJSON | ||
parseCreationOptionsFromJSON?( | ||
options: PublicKeyCredentialCreationOptionsJSON | ||
): PublicKeyCredentialCreationOptions | ||
// See https://w3c.github.io/webauthn/#sctn-parseRequestOptionsFromJSON | ||
parseRequestOptionsFromJSON?( | ||
options: PublicKeyCredentialRequestOptionsJSON | ||
): PublicKeyCredentialRequestOptions | ||
// See https://w3c.github.io/webauthn/#dom-publickeycredential-tojson | ||
toJSON?(): PublicKeyCredentialJSON | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's just so much going on here, and doesn't feel very used.
Can we just simplify this... the TypeScript compiler already includes all of the types necessary. We don't need to redefine anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entire section is done as sugar around the TypeScript compiler fields. In the TypeScript version, certain fields are BufferSource while in the sugar version suffixed with -JSON
these fields are cast to base64URL / string
which is slightly easier to work with.
I would also like to remove this if possible, will see if there's a clean workaround. In the meantime open to suggestions as well
What kind of change does this PR introduce?
We introduce bindings for MFA (WebAuthn) which consists of the following:
enroll()
a. Single Step -
enroll({factorType: 'webauthn'})
b. Multi-Step -
enroll(factorType: 'webauthn, webAuthn{...}
)challenge()
-verify()
a. Single Step -
verify({factorType: 'webauthn'})
b. Multi-Step -
verify({factorType: 'webauthn', webAuthn{..}})
We also expose a few helper methods:
identifyAuthenticationError
browserSupportsWebAuthn